Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graalvm-oracle: init at 22 #321026

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

graalvm-oracle: init at 22 #321026

wants to merge 1 commit into from

Conversation

farnoy
Copy link
Member

@farnoy farnoy commented Jun 19, 2024

Supersedes #259639, resolves #259631

Description of changes

This adds oracle-graalvm and make buildGraalvm more generic to be used on other places than community-edition.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Pinging @thiagokokada (reviewer from the previous PR)


Add a 👍 reaction to pull requests you find important.

graalvmCEPackages =
recurseIntoAttrs (callPackage ../development/compilers/graalvm/community-edition { });
graalvm-ce = graalvmCEPackages.graalvm-ce;
graalvm-oracle = callPackage ../development/compilers/graalvm/oracle { };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to be called GraalVM EE, now just "Oracle GraalVM". Maybe oracle-graalvm would be a better name here?

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 19, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024
@rafaelrc7 rafaelrc7 mentioned this pull request Jul 20, 2024
12 tasks
@rafaelrc7
Copy link
Member

Do you plan on adding the other supported versions by oracle's graalvm? Such as for JDK 17 and 21.

@farnoy
Copy link
Member Author

farnoy commented Jul 25, 2024

@rafaelrc7 I don't have any use for them but I'll take a look

@farnoy
Copy link
Member Author

farnoy commented Jul 25, 2024

Rebased on latest nixpkgs to resolve conflicts

@rafaelrc7
Copy link
Member

@rafaelrc7 I don't have any use for them but I'll take a look

Thanks! I am currently working on packaging pkl in #286658 and the native build depends on graalvm 17, so it would be a lot of help.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 25, 2024
Comment on lines 15882 to 15883
graalvmCEPackages = callPackage ../development/compilers/graalvm/community-edition { };
graalvmPackages = recurseIntoAttrs (callPackage ../development/compilers/graalvm { });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that we have 2 different graalvmPackages.

I think since we are introducing Oracle GraamVM again, it is a good time to move everything to graalvmPackages and make graalvmCEPackages an alias to graalvmPackages, and deprecate and eventually remove graalvmCEPackages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that, but can you be more specific? Do we want graalvmPackages to temporarily be the CE packages and also graalvmPackages.oracle + graalvmPackages.ce as the long-term, stable names?

Do we also want this nesting for JDK packages or is pkgs.oracle-graalvm fine as it is in the current version of the PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that, but can you be more specific? Do we want graalvmPackages to temporarily be the CE packages and also graalvmPackages.oracle + graalvmPackages.ce as the long-term, stable names?

Yes. But probably the names will be graalvmPackages.graalvm-oracle and graalvmPackages.graalvm-ce, because there are other packages inside graalvmCEPackages too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR, please let me know what you think

"hashes" = {
"aarch64-linux" = {
sha256 = "1s3ma7x22wqg10cqhdgwj0g9qk3n7bqy10dq6x1vgpljdd41v6dx";
url = "https://download.oracle.com/graalvm/17/latest/graalvm-jdk-17_linux-aarch64_bin.tar.gz";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this URL is deterministic, there is no /archive/ version for these

url = "https://download.oracle.com/graalvm/22/archive/graalvm-jdk-22_linux-aarch64_bin.tar.gz";
};
"x86_64-linux" = {
sha256 = "d1860b5b7588310e70b259c891156f6d0cbc34d0d1feec3b37169ed2a415f3c3";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these hashes are correct, I calculated them with nix-prefetch-url but I need to double check.

pkgs/by-name/gr/graalvm-ce/package.nix Outdated Show resolved Hide resolved
@farnoy
Copy link
Member Author

farnoy commented Aug 9, 2024

Thanks for the feedback @chayleaf , updated

pkgs/development/compilers/graalvm/default.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/graalvm/default.nix Outdated Show resolved Hide resolved
Comment on lines 565 to 566
graalvmCEPackages = graalvmPackages;
graalvm-ce = graalvmPackages.graalvm-ce;
Copy link
Contributor

@chayleaf chayleaf Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
graalvmCEPackages = graalvmPackages;
graalvm-ce = graalvmPackages.graalvm-ce;
graalvmCEPackages = graalvmPackages; # 2024-08-10

aliases.nix is for deprecated aliases, so you have to put the date in there as a rough estimate of whether it's okay to replace the alias with a throw (see maintainers/scripts/remove-old-aliases.py)

graalvm-ce is probably fine to not deprecate? in that case, just put it back to all-packages.nix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

graalvm-ce now lives in graalvmPackages.graalvm-ce so I'm deprecating this old name #321026 (comment)

"22" = {
"aarch64-linux" = {
sha256 = "sha256-0wPTDBB2T+qrB27+eQGd88mBsEh/ysPfelMT929hBA4=";
url = "https://download.oracle.com/graalvm/22/archive/graalvm-jdk-22_linux-aarch64_bin.tar.gz";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those URLe are unacceptable, especially in this case since Hydra will not cache the result (because this is unfree).

We need either to have a stable URL from upstream or have the files archived somewhere (e.g.: archive.org).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some precedents, but they typically result in the packages being marked as broken (see fmod)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found more stable links (with full version strings) here.

I'm not sure if this is sufficient from what you're saying. This package will always be unfree, regardless where we download it from? And I'm not sure it's legal to replicate it to archive.org.

Copy link
Contributor

@chayleaf chayleaf Aug 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ones with full version strings should be sufficient (there are some for 17 as well)

@farnoy
Copy link
Member Author

farnoy commented Aug 11, 2024

Updated download links to those for a specific patch version of each package.

@@ -15879,8 +15879,7 @@ with pkgs;
openjdk = jdk;
openjdk_headless = jdk_headless;

graalvmCEPackages = callPackage ../development/compilers/graalvm/community-edition { };
graalvm-ce = graalvmCEPackages.graalvm-ce;
graalvmPackages = recurseIntoAttrs (callPackage ../development/compilers/graalvm { });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the usage of recurseIntoAttrs here, we probably need to introduce a scope inside graalvmPackages to ensure that all packages are build with the same scope.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with scopes and not sure what this means. I tried just removing recurseIntoAttrs and things seem to work all the same, so it seems it can be safely dropped?

Is the fact that pkgs/development/compilers/graalvm/graalvm-oracle/default.nix uses graalvmPackages.buildGraalvm (from the parent "scope"?) problematic?

@farnoy
Copy link
Member Author

farnoy commented Sep 2, 2024

Anything I should do to push this forward?

@thiagokokada
Copy link
Contributor

@ofborg build babashka clojure-lsp

@thiagokokada
Copy link
Contributor

Build broke for graalvm in darwin. Need to investigate if this is something that is currently broken in master or is something that this branch is doing.

@farnoy
Copy link
Member Author

farnoy commented Sep 8, 2024

I've had a friend build clojure-lsp and babashka off this branch on aarch64-darwin, and it worked. Not sure what's going wrong when ofborg does it?

nix run github:nixos/nixpkgs#nix-info -- -m
 - system: `"aarch64-darwin"`
 - host os: `Darwin 23.5.0, macOS 14.5`
 - multi-user?: `yes`
 - sandbox: `no`
 - version: `nix-env (Nix) 2.24.5`
 - channels(root): `"nixpkgs"`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixpkgs`

Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash your commits accordingly to CONTRIBUTING.md guidelines.

@farnoy farnoy force-pushed the oracle-graalvm branch 2 times, most recently from 04d1761 to cec132e Compare September 23, 2024 09:16
@ofborg ofborg bot requested a review from thiagokokada September 23, 2024 10:12
@farnoy
Copy link
Member Author

farnoy commented Sep 23, 2024

@thiagokokada Squashed and updated the commit author to myself

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024
@farnoy
Copy link
Member Author

farnoy commented Oct 1, 2024

Resolved conflicts again

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 1, 2024
@farnoy
Copy link
Member Author

farnoy commented Oct 11, 2024

Friendly ping :)

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: Oracle Graalvm 21
5 participants